fix(evaluation): return per-request error responses in Batch - #6262
fix(evaluation): return per-request error responses in Batch#6262AruneshDwivedi wants to merge 2 commits into
Conversation
dosubot
Bot
added
the
size:M
|
👋 Hi @AruneshDwivedi! Thanks for your contribution to this project. It looks like one or more of your commits are missing a DCO (Developer Certificate of Origin) sign-off. The DCO is a simple way for you to certify that you have the right to submit this code under the project's license. How to fix this: # For future commits, use the -s flag
git commit -s -m "Your commit message"
# To sign off on existing commits in this PR
git rebase HEAD~$(git rev-list --count origin/v2..HEAD) --signoff
git push --force-with-leaseThe 📋 View the failing DCO check for more details For more information about the DCO, visit: https://developercertificate.org/ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: request changes
The PR correctly aims to return per-request errors in Batch, but the current implementation maps every failure—including internal store errors, evaluation failures, and unknown flag types—to NOT_FOUND, which masks real problems and misleads callers. These concerns were raised in the previous automated review and remain unaddressed. Once not-found is distinguished from other errors and the reasons are corrected, this should be good to go.
internal/server/evaluation/evaluation.go
- major (L502): All
store.GetFlagerrors—including internal/database failures—are mapped to a per-requestNOT_FOUNDresponse, masking real system problems. Only true not-found errors should become per-requestNOT_FOUNDresponses; other errors must still abort the batch so they are observable and retryable. Useerrs.AsMatch[errs.ErrNotFound](err)to distinguish not-found errors, and returnnil, errfor everything else. - minor (L522): After a flag is successfully retrieved, failures from
s.boolean()ors.variant()are reported asNOT_FOUND, which is semantically wrong. ReturnUNKNOWN_ERROR_EVALUATION_REASONfor unexpected evaluation failures, or continue propagating the error instead of swallowing it. - minor (L588): An unknown flag type is reported as
NOT_FOUND, butGetFlagalready succeeded. UseUNKNOWN_ERROR_EVALUATION_REASONhere, or retain the batch-levelerrs.ErrInvalidfsince an invalid type indicates a programming error rather than a missing resource.
internal/server/evaluation/evaluation_test.go
- minor (L1121):
TestBatch_InternalError_GetFlagnow asserts that internal/database errors return a per-requestNOT_FOUNDresponse with no top-level error. Once non-not-foundGetFlagerrors are propagated again as above, restorerequire.Error(t, err)so the test verifies that internal failures still abort the batch.
🤖 Automated review by the Flipt PR review agent.
…ng batch Removed unused 'errors' import and updated affected unit tests. Signed-off-by: Arunesh Dwivedi <arunesh.devops@gmail.com>
0f01fd9 to
f0f33ad
Compare
|
👋 Hi maintainers! Just pushed an updated version of this PR with:
This replaces #6256 (which had a stale merge commit causing DCO failure). Would appreciate CI validation and review! 🙏 |
|
Hey maintainers! Updated PR with DCO sign-off and all fixes applied. Replaces #6256 which had stale merge commits causing DCO failure. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2 #6262 +/- ##
==========================================
- Coverage 62.37% 62.30% -0.07%
==========================================
Files 145 145
Lines 14867 14895 +28
==========================================
+ Hits 9273 9281 +8
- Misses 4833 4853 +20
Partials 761 761
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
hey @AruneshDwivedi. Thanks for your PR. Could you please rebase your branch onto the latest upstream v2 branch when you get a chance? Also, if possible, could you take another look at the issues reported by the review agent? If you agree with the feedback, it would be great to address it. If you think any of the findings are false positives, a short comment explaining why would be really helpful for us. |
Signed-off-by: Arunesh Dwivedi <arunesh.devops@gmail.com>
|
Hey flipt maintainers! 👋\n\nThis PR fixes the batch evaluation endpoint to return per-request error responses instead of aborting the entire batch on individual flag lookup failures. Changes include:\n\n1. Remove unused 'errors' import\n2. Convert error returns to per-request error responses in the Batch endpoint\n3. Updated unit tests to verify per-request error responses\n4. All gofmt and lint rules pass (require.NoError for test assertions)\n\nDCO sign-off included. All lightweight checks pass.\n\nReplaces #6256 (which had stale merge commits causing DCO failure).\n\nWould appreciate review when convenient! 🙏 |
2 participants
fix(evaluation): return per-request error responses in Batch instead of aborting entire request
The Batch evaluation endpoint previously returned errors for individual flag
lookup failures and unknown flag types, killing the entire batch. Changed to
return per-request error responses so other evaluations in the batch continue
processing.
Removed unused 'errors' import and updated affected unit tests.